Include pending HTLC's in ChannelDetails #2442

wvanlint · 2023-07-21T22:44:42Z

This exposes details around pending HTLCs in ChannelDetails. The state of the HTLC in the state machine is also included, so it can be determined which protocol message the HTLC is waiting for to advance.

codecov-commenter · 2023-07-22T00:16:49Z

Codecov Report

Attention: 59 lines in your changes are missing coverage. Please review.

Comparison is base (e64342a) 89.18% compared to head (67e788e) 89.11%.

Files	Patch %	Lines
lightning/src/ln/channel.rs	61.06%	50 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	25.00%	6 Missing ⚠️
lightning/src/routing/router.rs	50.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2442      +/-   ##
==========================================
- Coverage   89.18%   89.11%   -0.07%     
==========================================
  Files         115      115              
  Lines       93379    93522     +143     
  Branches    93379    93522     +143     
==========================================
+ Hits        83276    83345      +69     
- Misses       7575     7643      +68     
- Partials     2528     2534       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-07-26T21:41:46Z

lightning/src/ln/channel.rs

+			dust_buffer_feerate * htlc_success_tx_weight(self.get_channel_type()) / 1000
+		};
+		let holder_dust_limit_success_sat = htlc_success_dust_limit + self.holder_dust_limit_satoshis;
+		for ref htlc in self.pending_inbound_htlcs.iter() {


I'm not sure about the rationale for not including claimed/failed holding cell HTLCs. It seems like we should include them (and indicate whether they're successful or failed), and maybe add a note in ChannelDetails::balance_msat about the considerations from the PR description, if necessary?

Thinking about it more, I think there's two approaches we can take to pending HTLCs and balances if we want a current view independent from commitment transactions:

Being as conservative as possible and considering as many pending HTLCs as encumbering the balance as possible. The pending HTLCs will therefore include both added holding cell HTLCs on one end and HTLCs being partially resolved on the other end. These pending HTLCs will subtract from the balance. Resolutions (claimed/failed) in the holding cell won't be considered.

Being as aggressive as possible and considering as many HTLCs as resolved as possible and removing them from the pending HTLCs and adjusting the balance appropriately. The pending HTLCs will therefore not include HTLCs that are in a partially resolved state or for which the resolution is in the holding cell. One question here is around partially failed HTLCs where one party can still broadcast a previous state and fulfill the HTLC.

Currently, we are very close to being fully consistent with the former, conservative option, minus the inclusion of inbound-claimed HTLCs in AvailableBalances::balance_msat due to https://github.com/lightningdevkit/rust-lightning/pull/1268/files.

However, the root cause of the underflow that was the motivation of that PR does not exist anymore. At the time of the PR, the amount in send_htlc was verified against the CommitmentStats (and this commitment transaction resolves the pending HTLC in the InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) state). Currently, we're verifying against AvailableBalances::next_outbound_htlc_limit_msat which does not resolve such pending HTLCs. I copied and reran the test in #1268 from the main branch and it is not possible to send an HTLC back anymore and cause an underflow (sending the return payment fails with Cannot send more than our next-HTLC maximum). This is likely why the test has been removed.

Therefore, I want to propose the conservative approach and remove the inclusion of inbound-claimed HTLCs in AvailableBalances::balance_msat. This will also cause the sum of the reserves, inbound capacity, outbound capacity and pending HTLCs to be equal to the channel size when the reserves are met. What do you think @valentinewallace @TheBlueMatt?

wvanlint · 2023-08-05T22:59:08Z

After discussing offline, there were the following conclusions:

Many use cases can be addressed with ChannelMonitor::get_claimable_balances. This should likely be used over ChannelDetails::balance_msat.
Other use cases include debugging, and can vary whether they should look at holding cells or not.

I updated the PR to expose all the information Channel has around pending HTLC's including HTLC state and holding cell state (and whether the HTLC is dust) without any further processing. Downstream users can then do debugging or any computation they would like.

One implementation alternative I considered here was reusing the existing types (InboundHTLCOutput, OutboundHTLCOutput, HTLCUpdateAwaitingACK) as much as possible but this results in cascading requirements on various internal types to set visibilities to public and implement Clone and Writeable. Let me know if that is preferable though.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-08-08T22:01:24Z

lightning/src/ln/channel.rs

+	RemoteAnnouncedFail,
+	AwaitingRemoteRevokeToAnnounceForward,
+	AwaitingRemoteRevokeToAnnounceFail,
+	AwaitingAnnouncedRemoteRevokeForward,


Do you have a strong feeling on if we differentiate between the two awaiting-remote-revoke states? They're pretty different in the state machine itself, but do users really ever care whether we're waiting on the remote to go to state 2/3 or if we're in state 2/3?

If the use cases can vary and include debugging, I was thinking to expose all states that are available. What do you think? Would there be debugging use cases around stuck HTLCs?

Even for debugging I'm not sure there's a real difference here. Both indicate we're waiting on the peer to send an RAA for a previous state, the exact place we are in the state machine isn't all that interesting, at some point you go look at logs (or, really, once you see you're just waiting on an RAA for a while you can yell at the other peer).

Collapsed these states where we are waiting for remote revoke_and_ack's.

TheBlueMatt · 2023-08-08T22:07:50Z

lightning/src/ln/channel.rs

@@ -152,6 +152,50 @@ enum InboundHTLCState {
 	LocalRemoved(InboundHTLCRemovalReason),
 }

+#[derive(Clone, Debug, PartialEq)]
+pub enum InboundHTLCStateDetails {


Once we make this pub we'll need to write a ton of docs - IMO each variant should have, at least, a description of whether this HTLC is included in the current local commitment transaction (ie the one signed by our peer which we can broadcast at any time) and whether it will be included in the next remote commitment transaction (ie the next thing we need to sign that determines if we can send another HTLC or not).

Added documentation here based on InboundHTLCState and OutboundHTLCState.

TheBlueMatt · 2023-08-08T22:12:58Z

lightning/src/ln/channel.rs

+	RemoteRemovedFailure,
+	AwaitingRemoteRevokeToRemoveSuccess,
+	AwaitingRemoteRevokeToRemoveFailure,
+	AwaitingRemovedRemoteRevokeSuccess,


Same here, do we want to merge the AwaitingRemoteRevoke... states?

Collapsed these states where we are waiting for remote revoke_and_ack's.

TheBlueMatt · 2023-08-08T22:13:09Z

lightning/src/ln/channel.rs

+	LocalAnnounced,
+	Committed,
+	RemoteRemovedSuccess,
+	RemoteRemovedFailure,


I wonder if we want to drop the RemoteRemoved... states - they're really just the remote side indicating that they wish to remove, but they haven't actually committed to it yet. The states shouldn't hang around long, so I dunno if they really communicate much additional information.

Collapsed this state into Committed.

TheBlueMatt · 2023-08-08T22:14:17Z

lightning/src/ln/channel.rs

+#[derive(Clone, Debug, PartialEq)]
+pub enum InboundHTLCStateDetails {
+	RemoteAnnouncedForward,
+	RemoteAnnouncedFail,


Same as on the outbound case - not sure if RemoteAnnounced... adds much value to expose.

I kept this state around as otherwise the pending HTLC would be dropped.

I'm still a bit dubious of it because I anticipate no one will ever see this - generally nodes send update_add_htlc and then commitment_signed immediately, at which point we'll have already failed this HTLC back and we'll be in another state. Still, if we keep it we need to describe it in way more detail - something about how "we intend to immediately fail the HTLC" and what cases this can arise in.

I think I mainly wanted the state details to be a .map() of internal state instead of a .filter_map(). Added documentation around RemoteAnnounced states. Alternatively, I can collapse this into the subsequent state.

I don't quite get why - this is a public interface that should be easy for developers to deal with, there's no specific requirement that it be 1:1 with the internal state, and not being 1:1 forces developers to handle cases that they might otherwise not care about. Can you provide a bit more detail on why you want it to be 1:1?

We'll also, now, want to drop the *Forward/*Fail distinction, I think - post #2845 it'll go away in future HTLCs, so exposing it via the public interface doesn't make much sense.

One of the reasons for adding this PR was also observability, so I was leaning towards exposing information we have, especially since the states are caused by and are dependent on external messages. It might for example be possible to get stuck at the RemoteAnnounced state if the counterparty does not send commitment_signed due to an implementation bug.

I agree that information on announcements adds less value than the rest though and I probably should be consistent with #2442 (comment) as well, added a commit to not expose this.

Dropped the *Forward/*Fail distinction as well.

TheBlueMatt · 2023-08-08T22:16:35Z

One implementation alternative I considered here was reusing the existing types (InboundHTLCOutput, OutboundHTLCOutput, HTLCUpdateAwaitingACK) as much as possible but this results in cascading requirements on various internal types to set visibilities to public and implement Clone and Writeable. Let me know if that is preferable though.

I like it the way you have it - having different public types gives us the ability to hide some nuance that users may not need to care about.

TheBlueMatt · 2023-08-08T22:17:52Z

lightning/src/ln/channel.rs

@@ -249,6 +371,39 @@ enum HTLCUpdateAwaitingACK {
 	},
 }

+#[derive(Clone, Debug, PartialEq)]
+pub enum HTLCUpdateAwaitingACKDetails {
+	AddHTLC {


IMO we should just include this in the outbound HTLC set with a different state flag.

Moved holding cell updates as separate states into pending inbound and outbound HTLCs.

TheBlueMatt · 2023-08-08T22:18:12Z

lightning/src/ln/channel.rs

+		skimmed_fee_msat: Option<u64>,
+		is_dust: bool,
+	},
+	ClaimHTLC {


I wonder if we shouldn't just include this and fail in the inbound HTLC sets as a bool indicating our intent to remove.

I agree, I think we can get rid of holding_cell_updates and not reference the holding cell explicitly.

Moved holding cell updates as separate states into pending inbound and outbound HTLCs.

lightning/src/ln/channel.rs

valentinewallace · 2023-08-08T22:44:53Z

lightning/src/ln/channel.rs

+		skimmed_fee_msat: Option<u64>,
+		is_dust: bool,
+	},
+	ClaimHTLC {


I agree, I think we can get rid of holding_cell_updates and not reference the holding cell explicitly.

TheBlueMatt

Mostly some comments on the docs - at a high level we try hard to avoid writing docs which assume users of LDK understand the inner workings of the lightning state machine. Also, all documentation should strive to include both a what but also a why you'd care about this and a how you'd use this information (at least for things that aren't super duper obvious).

TheBlueMatt · 2023-08-16T19:58:41Z

lightning/src/ln/channel.rs

+#[derive(Clone, Debug, PartialEq)]
+pub enum InboundHTLCStateDetails {
+	RemoteAnnouncedForward,
+	RemoteAnnouncedFail,


I'm still a bit dubious of it because I anticipate no one will ever see this - generally nodes send update_add_htlc and then commitment_signed immediately, at which point we'll have already failed this HTLC back and we'll be in another state. Still, if we keep it we need to describe it in way more detail - something about how "we intend to immediately fail the HTLC" and what cases this can arise in.

TheBlueMatt · 2023-08-16T19:59:59Z

lightning/src/ln/channel.rs

+	/// We are awaiting the appropriate revoke_and_ack's from the remote before this HTLC is included
+	/// on the remote commitment transaction, possibly for a prior state first.
+	AwaitingRemoteRevokeToAddForward,
+	/// See above.


Same here, at least the documentation part. I think I agree that we should keep this one, because it at least sticks around longer that people might see it, but if the documentation is "see above" that probably implies we shouldn't have it - we're telling users to handle it the same as the other case :).

Added documentation on this state around the intent to fail the HTLC.

TheBlueMatt · 2023-08-16T20:00:42Z

lightning/src/ln/channel.rs

+	/// transactions, but we are awaiting the appropriate revoke_and_ack's to remove this HTLC from
+	/// the remote commitment transaction, possibly for a prior state first.
+	AwaitingRemoteRevokeToRemoveFulfill,
+	/// See above.


Same here, on the documentation end.

Added documentation on this state around the intent to fail the HTLC.

TheBlueMatt · 2023-08-16T20:01:58Z

lightning/src/ln/channel.rs

+	/// See above.
+	AwaitingRemoteRevokeToAddFail,
+	/// Committed indicates that this HTLC has been included in the commitment_signed and
+	/// revoke_and_ack flow on both sides and is included in both commitment transactions.


Can we mention something about how the HTLC may have been forwarded to another channel, or may be awaiting such forwarding or claim by us, or other parts for an MPP payment?

TheBlueMatt · 2023-08-16T20:03:02Z

lightning/src/ln/channel.rs

+/// Exposes details around pending inbound HTLCs.
+#[derive(Clone, Debug, PartialEq)]
+pub struct InboundHTLCDetails {
+	/// The corresponding HTLC ID.


We should describe how someone might use this - something about once its past RemoteAnnounced* this is unique for this channel and no further inbound HTLCs will have the same ID. If an HTLC only appears as announced we may have a collision, and outbound HTLCs have a separate ID namespace.

Added. I think this will be useful as well if there are ever HTLC-specific (analytics) events that can be correlated by HTLC id.

TheBlueMatt · 2023-08-16T20:05:09Z

lightning/src/ln/channel.rs

+	/// The state of the HTLC in the update_*_htlc, commitment_signed, revoke_and_ack flow.
+	/// Informs on which commitment transactions the HTLC is included.
+	pub state: InboundHTLCStateDetails,
+	/// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed


Something about burning to fee on closure?

TheBlueMatt · 2023-08-16T20:06:02Z

lightning/src/ln/channel.rs

+	pub cltv_expiry: u32,
+	/// The payment hash.
+	pub payment_hash: PaymentHash,
+	/// The state of the HTLC in the update_*_htlc, commitment_signed, revoke_and_ack flow.


Should we just say something about the HTLC's state machine? I think this is the only place in the public documentation that we mention the state machine in terms of the messages being exchanged.

I described the state machine in more detail in InboundHTLCStateDetails and OutboundHTLCStateDetails and left a reference here.

TheBlueMatt · 2023-08-16T20:06:55Z

lightning/src/ln/channel.rs

@@ -152,6 +152,72 @@ enum InboundHTLCState {
 	LocalRemoved(InboundHTLCRemovalReason),
 }

+/// Exposes the state of pending inbound HTLCs.
+///


Can we get a brief description of the HTLC state machine at a high level? I dont think we have anywhere else in the API which describes it, and we generally try to avoid APIs which assume knowledge of how lightning works under the hood.

TheBlueMatt · 2023-08-16T20:09:24Z

lightning/src/ln/channel.rs

+	AwaitingRemoteRevokeToAdd,
+	/// The Committed state indicates that this HTLC is included on the remote's commitment
+	/// transaction. This includes the subsequent state where we include it in the local commitment
+	/// transaction, as:


This part is super confusing to me - I can figure out what you mean by reading the code (its a reference to the remote party having indicated intent to remove the HTLC, but not having yet committed to it), but its too verbose and assumes some level of knowing the HTLC state machine (to figure out what "the subsequent state" is).

I trimmed this down. It was mainly copied from

rust-lightning/lightning/src/ln/channel.rs

Lines 171 to 182 in 6b0ba8c

/// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we

/// created it we would have put it in the holding cell instead). When they next revoke_and_ack

/// we will promote to Committed (note that they may not accept it until the next time we

/// revoke, but we don't really care about that:

/// * they've revoked, so worst case we can announce an old state and get our (option on)

/// money back (though we won't), and,

/// * we'll send them a revoke when they send a commitment_signed, and since only they're

/// allowed to remove it, the "can only be removed once committed on both sides" requirement

/// doesn't matter to us and it's up to them to enforce it, worst-case they jump ahead but

/// we'll never get out of sync).

/// Note that we Box the OnionPacket as it's rather large and we don't want to blow up

/// OutboundHTLCOutput's size just for a temporary bit

, but I agree this can be higher-level as it is user-facing documentation.

TheBlueMatt · 2023-08-16T20:10:32Z

lightning/src/ln/channel.rs

+	/// will be removed from the remote's commitmen transaction as well, possibly for a prior state
+	/// first.
+	AwaitingRemoteRevokeToRemoveSuccess,
+	/// See above.


Please at least copy the docs - not every language lists these things in order, and eg in an IDE you may load the documentation on the specific variant and be kinda lost.

wpaulino · 2023-10-31T16:30:13Z

Is this still on your radar @wvanlint?

wvanlint · 2023-11-03T00:28:58Z

@wpaulino Sorry, it dropped off my priority list for a bit while I was thinking about how to reword the documentation to not assume knowledge from the user and still be precise. I would like to wrap this up but I might not be able to get back to it in the short-term. Is this a blocker for anything?

wpaulino · 2023-11-03T01:17:44Z

Is this a blocker for anything?

Nope, just wondering :)

coderabbitai · 2024-01-22T01:50:23Z

Warning

Rate Limit Exceeded

@wvanlint has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 33 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between e64342a and 67e788e.

Walkthrough

The recent modifications enhance the Lightning network's channel management by introducing detailed tracking and management of pending inbound and outbound Hashed Time-Locked Contracts (HTLCs). This includes the establishment of new enums and structs to represent the states and details of HTLCs, alongside updates to serialization and testing frameworks to accommodate these changes. These enhancements aim to provide clearer insights into HTLC states, improving the network's reliability and diagnostic capabilities.

Changes

Files	Change Summaries
`fuzz/src/router.rs`	Initialized `pending_inbound_htlcs` and `pending_outbound_htlcs` as empty vectors in `do_test`.
`lightning/src/ln/channel.rs`	Introduced enums and structs for detailed HTLC state and details, added related functions.
`lightning/src/ln/channelmanager.rs`	Added public exports for HTLC details, updated `ChannelDetails`, implemented serialization.
`lightning/src/routing/router.rs`	Added `pending_inbound_htlcs` and `pending_outbound_htlcs` fields in tests and bench utils.

Poem

In the realm of lightning, a change does gleam,
🌩️ Where HTLCs flow like a swift-running stream.
A rabbit hopped by, with code in its trail,
🐇💻 Enhancing the network, without any fail.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6b0ba8c and 7a44af3.

Files selected for processing (4)

fuzz/src/router.rs (1 hunks)
lightning/src/ln/channel.rs (5 hunks)
lightning/src/ln/channelmanager.rs (6 hunks)
lightning/src/routing/router.rs (2 hunks)

Additional comments: 22

fuzz/src/router.rs (1)

245-246: Initialization of pending_inbound_htlcs and pending_outbound_htlcs as empty vectors is correct and aligns with the PR's objective to provide more granular information about the state of HTLCs.

lightning/src/routing/router.rs (2)

3288-3289: Initialization of pending_inbound_htlcs and pending_outbound_htlcs as empty vectors is correct and aligns with the PR's objective to track pending HTLCs without altering existing balances.

8431-8432: The addition of pending_inbound_htlcs and pending_outbound_htlcs to the Router struct in this context is consistent with the previous hunk and the PR's objectives.
lightning/src/ln/channel.rs (13)
161-175: The documentation provided for InboundHTLCStateDetails is comprehensive and aligns with the previous comments about needing a high-level description of the HTLC state machine. This should help users understand the lifecycle of an HTLC.

176-220: The enum InboundHTLCStateDetails includes detailed comments for each variant, which is good for clarity. However, ensure that the descriptions are accurate and reflect the actual behavior of the code. The comments from TheBlueMatt suggest that some states may not be seen often, such as RemoteAnnounced..., and if kept, they need detailed descriptions. Verify that these descriptions have been added and are sufficient.

222-247: The From trait implementation for InboundHTLCStateDetails looks correct and follows Rust conventions. It's good to see that the conversion logic is encapsulated in a way that keeps the code clean and maintainable.

249-257: The impl_writeable_tlv_based_enum! macro usage for InboundHTLCStateDetails is appropriate and follows the existing codebase patterns. Ensure that the TLV fields are correctly mapped and that no additional data is required for each state.

267-294: The InboundHTLCDetails struct is well-documented, with clear descriptions for each field. The inclusion of is_dust and its implications on the commitment transaction fee is particularly important for users to understand the financial consequences of channel operations.

296-303: The impl_writeable_tlv_based! macro for InboundHTLCDetails seems correct. Ensure that all required fields are present and that the TLV mapping is accurate.

338-372: The documentation for OutboundHTLCStateDetails is thorough and mirrors the structure provided for inbound HTLCs. This consistency is good for maintainability and understanding of the code. Ensure that the state descriptions are accurate and complete.

374-395: The From trait implementation for OutboundHTLCStateDetails is consistent with the inbound counterpart and follows Rust best practices. The handling of transient states is noted, which is important for understanding the state transitions.

397-402: The impl_writeable_tlv_based_enum! macro for OutboundHTLCStateDetails is used correctly. Confirm that the TLV fields are correctly mapped and that no additional data is required for each state.

442-473: The OutboundHTLCDetails struct is well-documented, and the inclusion of skimmed_fee_msat is a good addition for transparency in fee handling. The is_dust field is also important for understanding the financial implications of channel operations.

475-483: The impl_writeable_tlv_based! macro for OutboundHTLCDetails appears correct. Ensure that all required fields are present and that the TLV mapping is accurate.

2214-2253: The method get_pending_inbound_htlc_details is well-implemented, taking into account holding cell updates and dust limits. The logic for determining is_dust is clear and should work as intended. Ensure that the calculation of dust limits is correct and consistent with the rest of the codebase.

Verification successful

The rg command has provided the context around the calculation of holder_dust_limit_success_sat in the get_pending_inbound_htlc_details method. The calculation is consistent with the rest of the codebase. The variable holder_dust_limit_success_sat is computed based on htlc_success_dust_limit and self.holder_dust_limit_satoshis. The htlc_success_dust_limit is calculated based on the dust_buffer_feerate and self.get_channel_type(). The holder_dust_limit_success_sat is then used to determine whether an HTLC is considered dust.

The calculation of holder_dust_limit_success_sat is consistent with the rest of the codebase and should work as intended. Therefore, the logic for determining is_dust in the get_pending_inbound_htlc_details method is correct and consistent with the rest of the codebase.

The verification has provided the necessary information to confirm that the calculation of dust limits is correct and consistent with the rest of the codebase.

Conclusion

The calculation of dust limits in the get_pending_inbound_htlc_details method is consistent with the rest of the codebase. The logic for determining is_dust should work as intended.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that the calculation of dust limits in get_pending_inbound_htlc_details is consistent with the rest of the codebase.
rg --type rust --context 5 'holder_dust_limit_success_sat'
Length of output: 2640
* 2255-2296: The method `get_pending_outbound_htlc_details` mirrors the inbound method and handles holding cell updates appropriately. The calculation of `is_dust` is consistent with the inbound method, which is good for maintainability. Confirm that the dust limit calculation is correct and consistent.

Verification successful

The rg command has provided the context around the calculation of holder_dust_limit_timeout_sat in the get_pending_outbound_htlc_details method. The calculation of holder_dust_limit_timeout_sat is consistent with the rest of the codebase. It's derived from htlc_timeout_dust_limit and self.holder_dust_limit_satoshis, and it's used to determine whether an HTLC is considered dust.

The calculation is based on the amount of the HTLC and the holder_dust_limit_timeout_sat. If the amount of the HTLC is less than holder_dust_limit_timeout_sat, it's considered dust. The same calculation is used consistently throughout the method, ensuring maintainability and consistency.

The verification confirms that the dust limit calculation is correct and consistent with the rest of the codebase.

Conclusion

The calculation of the dust limit in the get_pending_outbound_htlc_details method is consistent with the rest of the codebase. Therefore, the review comment has been verified and the code is correct.

Verifying the review comment
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that the calculation of dust limits in get_pending_outbound_htlc_details is consistent with the rest of the codebase.
rg --type rust --context 5 'holder_dust_limit_timeout_sat'
Length of output: 4186
lightning/src/ln/channelmanager.rs (6)
47-47: The re-export of InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, and OutboundHTLCStateDetails is consistent with the PR's objective to make these types publicly available. This change aligns with the previous discussion and the decision to use re-exports.

1797-1804: The addition of pending_inbound_htlcs and pending_outbound_htlcs to the ChannelDetails struct is a significant change. It's important to ensure that all serialization and deserialization paths handle these new fields correctly, especially with the versioning comments indicating compatibility with specific LDK versions.

1882-1883: The implementation of get_pending_inbound_htlc_details and get_pending_outbound_htlc_details methods in the context object is correct and aligns with the PR's objectives. Ensure that these methods are covered by unit tests to validate their behavior.

9432-9433: The serialization logic for pending_inbound_htlcs and pending_outbound_htlcs using optional_vec is correct. It's important to ensure that the serialization logic is backward compatible and that there are tests to verify the serialization and deserialization processes.

Verification successful

The shell script output indicates that the serialization and deserialization tests for the new fields pending_inbound_htlcs and pending_outbound_htlcs are present in the following files:

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

The presence of these tests suggests that the serialization and deserialization processes for the new fields have been verified. Therefore, the changes in the serialization logic for pending_inbound_htlcs and pending_outbound_htlcs using optional_vec are covered by the tests.

The verification has provided evidence that the serialization and deserialization tests include the new fields, confirming that the serialization logic is backward compatible and that there are tests to verify the serialization and deserialization processes.

Conclusion

The verification has been successful, and the serialization and deserialization tests for the new fields are present in the relevant files.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that serialization and deserialization tests include the new fields.
rg --type rust 'optional_vec' --files-with-matches | xargs rg --type rust '#\[test\]'
Length of output: 6195
* 9472-9473: The deserialization logic for `pending_inbound_htlcs` and `pending_outbound_htlcs` is present. Ensure that there are tests for deserialization that include scenarios where these fields are not present, to maintain backward compatibility. * 9510-9511: Defaulting `pending_inbound_htlcs` and `pending_outbound_htlcs` to empty vectors if they are not present in the serialized data is a good approach for backward compatibility. Confirm that this behavior is tested, especially for data serialized with older versions of LDK.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6b0ba8c and 55335f1.

Files selected for processing (4)

fuzz/src/router.rs (1 hunks)
lightning/src/ln/channel.rs (5 hunks)
lightning/src/ln/channelmanager.rs (6 hunks)
lightning/src/routing/router.rs (2 hunks)

Files skipped from review as they are similar to previous changes (4)

fuzz/src/router.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/routing/router.rs

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6b0ba8c and 44920ff.

Files selected for processing (4)

fuzz/src/router.rs (1 hunks)
lightning/src/ln/channel.rs (5 hunks)
lightning/src/ln/channelmanager.rs (6 hunks)
lightning/src/routing/router.rs (2 hunks)

Files skipped from review as they are similar to previous changes (4)

fuzz/src/router.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/routing/router.rs

wvanlint · 2024-01-22T05:38:50Z

Apologies for the delay here. Observability into pending HTLCs came up recently again, so I will be picking this back up. Added more documentation throughout.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 4bab9c8 and 58e8e16.

Files selected for processing (4)

fuzz/src/router.rs (1 hunks)
lightning/src/ln/channel.rs (5 hunks)
lightning/src/ln/channelmanager.rs (6 hunks)
lightning/src/routing/router.rs (2 hunks)

Files skipped from review as they are similar to previous changes (4)

fuzz/src/router.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/routing/router.rs

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 58e8e16 and 98df707.

Files selected for processing (1)

lightning/src/ln/channel.rs (5 hunks)

Additional comments: 4

lightning/src/ln/channel.rs (4)

161-198: The documentation and implementation of InboundHTLCStateDetails provide a clear and detailed explanation of the HTLC state machine, which is crucial for understanding the lifecycle of HTLCs. However, ensure that the documentation is kept up-to-date with any future changes to the HTLC state machine to prevent confusion.

235-261: The InboundHTLCDetails struct is well-defined, with clear documentation for each field. This struct will be useful for downstream users to get detailed information about inbound HTLCs. Ensure that any changes to the HTLC lifecycle are reflected in the state field to maintain the accuracy of the information provided.

306-339: Similar to the inbound HTLCs, the OutboundHTLCStateDetails enum is well-documented and provides a comprehensive view of the outbound HTLC state machine. This consistency in handling both inbound and outbound HTLCs is good practice. Keep the documentation updated with any changes to the outbound HTLC state machine.

410-450: The OutboundHTLCDetails struct is well-structured and documented, providing essential information about outbound HTLCs. The optional htlc_id field and the inclusion of skimmed_fee_msat offer a nuanced view of the HTLCs, which is valuable for debugging and analysis. Ensure that the handling of None values for htlc_id is consistent and well-documented in the codebase.

coderabbitai · 2024-02-01T19:49:10Z

lightning/src/ln/channel.rs

+impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
+	fn from(state: &InboundHTLCState) -> Option<InboundHTLCStateDetails> {
+		match state {
+			InboundHTLCState::RemoteAnnounced(_) => None,
+			InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) =>
+				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd),
+			InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) =>
+				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd),
+			InboundHTLCState::Committed =>
+				Some(InboundHTLCStateDetails::Committed),
+			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) =>
+				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail),
+			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed(_)) =>
+				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail),
+			InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) =>
+				Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill),
+		}
+	}
+}
+
+impl_writeable_tlv_based_enum!(InboundHTLCStateDetails,
+	(0, AwaitingRemoteRevokeToAdd) => {},
+	(2, Committed) => {},
+	(4, AwaitingRemoteRevokeToRemoveFulfill) => {},
+	(6, AwaitingRemoteRevokeToRemoveFail) => {};
+);


The implementation of From<&InboundHTLCState> for Option<InboundHTLCStateDetails> is logically sound, providing a clear mapping from InboundHTLCState to InboundHTLCStateDetails. This abstraction allows for easier handling of HTLC states in the higher-level logic. Consider adding unit tests to cover all possible state transitions to ensure the correctness of this mapping.

Would you like me to help with adding unit tests for this conversion?

coderabbitai · 2024-02-01T19:49:10Z

lightning/src/ln/channel.rs

+impl From<&OutboundHTLCState> for OutboundHTLCStateDetails {
+	fn from(state: &OutboundHTLCState) -> OutboundHTLCStateDetails {
+		match state {
+			OutboundHTLCState::LocalAnnounced(_) =>
+				OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd,
+			OutboundHTLCState::Committed =>
+				OutboundHTLCStateDetails::Committed,
+			// RemoteRemoved states are ignored as the state is transient and the remote has not committed to
+			// the state yet.
+			OutboundHTLCState::RemoteRemoved(_) =>
+				OutboundHTLCStateDetails::Committed,
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) =>
+				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess,
+			OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Failure(_)) =>
+				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFailure,
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) =>
+				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess,
+			OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Failure(_)) =>
+				OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFailure,
+		}
+	}
+}
+
+impl_writeable_tlv_based_enum!(OutboundHTLCStateDetails,
+	(0, AwaitingRemoteRevokeToAdd) => {},
+	(2, Committed) => {},
+	(4, AwaitingRemoteRevokeToRemoveSuccess) => {},
+	(6, AwaitingRemoteRevokeToRemoveFailure) => {};


The conversion from OutboundHTLCState to OutboundHTLCStateDetails is implemented correctly. As with the inbound HTLC states, consider adding unit tests for these conversions to ensure their correctness across all possible state transitions.

Would you like assistance in creating unit tests for these state conversions?

coderabbitai · 2024-02-01T19:49:10Z

lightning/src/ln/channel.rs

+	/// Returns information on all pending inbound HTLCs.
+	pub fn get_pending_inbound_htlc_details(&self) -> Vec<InboundHTLCDetails> {
+		let mut holding_cell_states = HashMap::new();
+		for holding_cell_update in self.holding_cell_htlc_updates.iter() {
+			match holding_cell_update {
+				HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => {
+					holding_cell_states.insert(
+						htlc_id,
+						InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill,
+					);
+				},
+				HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
+					holding_cell_states.insert(
+						htlc_id,
+						InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail,
+					);
+				},
+				HTLCUpdateAwaitingACK::FailMalformedHTLC { htlc_id, .. } => {
+					holding_cell_states.insert(
+						htlc_id,
+						InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail,
+					);
+				},
+				_ => {},
+			}
+		}
+		let mut inbound_details = Vec::new();
+		let htlc_success_dust_limit = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+			0
+		} else {
+			let dust_buffer_feerate = self.get_dust_buffer_feerate(None) as u64;
+			dust_buffer_feerate * htlc_success_tx_weight(self.get_channel_type()) / 1000
+		};
+		let holder_dust_limit_success_sat = htlc_success_dust_limit + self.holder_dust_limit_satoshis;
+		for htlc in self.pending_inbound_htlcs.iter() {
+			if let Some(state_details) = (&htlc.state).into() {
+				inbound_details.push(InboundHTLCDetails{
+					htlc_id: htlc.htlc_id,
+					amount_msat: htlc.amount_msat,
+					cltv_expiry: htlc.cltv_expiry,
+					payment_hash: htlc.payment_hash,
+					state: holding_cell_states.remove(&htlc.htlc_id).unwrap_or(state_details),
+					is_dust: htlc.amount_msat / 1000 < holder_dust_limit_success_sat,
+				});
+			}
+		}
+		inbound_details
+	}
+
+	/// Returns information on all pending outbound HTLCs.
+	pub fn get_pending_outbound_htlc_details(&self) -> Vec<OutboundHTLCDetails> {
+		let mut outbound_details = Vec::new();
+		let htlc_timeout_dust_limit = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+			0
+		} else {
+			let dust_buffer_feerate = self.get_dust_buffer_feerate(None) as u64;
+			dust_buffer_feerate * htlc_success_tx_weight(self.get_channel_type()) / 1000
+		};
+		let holder_dust_limit_timeout_sat = htlc_timeout_dust_limit + self.holder_dust_limit_satoshis;
+		for htlc in self.pending_outbound_htlcs.iter() {
+			outbound_details.push(OutboundHTLCDetails{
+				htlc_id: Some(htlc.htlc_id),
+				amount_msat: htlc.amount_msat,
+				cltv_expiry: htlc.cltv_expiry,
+				payment_hash: htlc.payment_hash,
+				skimmed_fee_msat: htlc.skimmed_fee_msat,
+				state: (&htlc.state).into(),
+				is_dust: htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat,
+			});
+		}
+		for holding_cell_update in self.holding_cell_htlc_updates.iter() {
+			if let HTLCUpdateAwaitingACK::AddHTLC {
+				amount_msat,
+				cltv_expiry,
+				payment_hash,
+				skimmed_fee_msat,
+				..
+			} = *holding_cell_update {
+				outbound_details.push(OutboundHTLCDetails{
+					htlc_id: None,
+					amount_msat: amount_msat,
+					cltv_expiry: cltv_expiry,
+					payment_hash: payment_hash,
+					skimmed_fee_msat: skimmed_fee_msat,
+					state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd,
+					is_dust: amount_msat / 1000 < holder_dust_limit_timeout_sat,
+				});
+			}
+		}
+		outbound_details
+	}


The methods get_pending_inbound_htlc_details and get_pending_outbound_htlc_details are crucial for retrieving detailed information about pending HTLCs. The logic appears sound, but it's important to ensure that these methods are covered by comprehensive tests, especially to verify the correct handling of dust limits and the mapping of HTLC states.

Would you like me to help with adding tests for these methods to ensure they handle all edge cases correctly?

TheBlueMatt

Logic and variants all LGTM! A few small nits on documentation and whatnot, but feel free to address those + squash and I think we can land this 🎉

TheBlueMatt · 2024-02-05T22:52:37Z

lightning/src/ln/channel.rs

+	/// The state of the HTLC in the state machine.
+	/// Determines on which commitment transactions the HTLC is included and what message the HTLC is
+	/// waiting for to advance to the next state.
+	/// See [InboundHTLCStateDetails] for information on the specific states.


nit

Suggested change

/// See [InboundHTLCStateDetails] for information on the specific states.

/// See [`InboundHTLCStateDetails`] for information on the specific states.

TheBlueMatt · 2024-02-05T22:53:12Z

lightning/src/ln/channel.rs

+	pub state: InboundHTLCStateDetails,
+	/// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed
+	/// from the local commitment transaction and added to the commitment transaction fee.
+	/// This takes into account the second-stage HTLC transactions as well.


Suggested change

/// This takes into account the second-stage HTLC transactions as well.

/// For non-anchor channels, this takes into account the cost of the second-stage HTLC transactions as well.

TheBlueMatt · 2024-02-05T22:54:00Z

lightning/src/ln/channel.rs

+	}
+}
+
+impl_writeable_tlv_based_enum!(InboundHTLCStateDetails,


Lets use the upgradeable version for all the new serialization.

Suggested change

impl_writeable_tlv_based_enum!(InboundHTLCStateDetails,

impl_writeable_tlv_based_enum_upgradable!(InboundHTLCStateDetails,

Done. This changes the field into Option<_> right?

TheBlueMatt · 2024-02-05T22:54:56Z

lightning/src/ln/channel.rs

+/// This can be used to inspect what next message an HTLC is waiting for to advance its state.
+#[derive(Clone, Debug, PartialEq)]
+pub enum OutboundHTLCStateDetails {
+	/// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is be added


Suggested change

/// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is be added

/// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is added

TheBlueMatt · 2024-02-05T22:57:37Z

lightning/src/ln/channel.rs

+	/// and we removed the HTLC from our commitment transaction through a commitment_signed and
+	/// revoke_and_ack exchange. We are awaiting the appropriate revoke_and_ack's from the remote for


Here and below, we might not actually have sent the CS yet, if we're waiting on the remote to revoke their latest state.

Suggested change

/// and we removed the HTLC from our commitment transaction through a commitment_signed and

/// revoke_and_ack exchange. We are awaiting the appropriate revoke_and_ack's from the remote for

/// and we removed the HTLC from our commitment transaction through a revoke_and_ack.

/// We are awaiting the appropriate revoke_and_ack's from the remote for

Ah, the exchange I mentioned here is receiving the commitment_signed message from the counterparty and returning the revoke_and_ack message. Added clarification by explicitly mentioning sending and receiving.

TheBlueMatt · 2024-02-05T22:58:14Z

lightning/src/ln/channel.rs

+	/// The state of the HTLC in the state machine.
+	/// Determines on which commitment transactions the HTLC is included and what message the HTLC is
+	/// waiting for to advance to the next state.
+	/// See [OutboundHTLCStateDetails] for information on the specific states.


nit

Suggested change

/// See [OutboundHTLCStateDetails] for information on the specific states.

/// See [`OutboundHTLCStateDetails`] for information on the specific states.

TheBlueMatt · 2024-02-05T22:58:47Z

lightning/src/ln/channel.rs

+	pub skimmed_fee_msat: Option<u64>,
+	/// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed
+	/// from the local commitment transaction and added to the commitment transaction fee.
+	/// This takes into account the second-stage HTLC transactions as well.


Suggested change

/// This takes into account the second-stage HTLC transactions as well.

/// For non-anchor channels, this takes into account the cost of the second-stage HTLC transactions as well.

TheBlueMatt · 2024-02-05T23:00:32Z

lightning/src/ln/channel.rs

+						InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail,
+					);
+				},
+				_ => {},


nit: make the ignored branch explicit with HTLCUpdateAwaitingACK::AddHTLC given there's only one option left and its nice to force ourselves to check the code if we ever add a new variant.

TheBlueMatt · 2024-02-05T23:01:43Z

lightning/src/ln/channel.rs

+	/// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed
+	/// from the local commitment transaction and added to the commitment transaction fee.
+	/// This takes into account the second-stage HTLC transactions as well.
+	///


Is it worth noting here (and in the inbound field for the same) that an HTLC could be dust for our commitment tx but not our counterparty's commitment tx or vice versa?

Added a note that this is specific to each party for the inbound and outbound case.

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e64342a and bb85b33.

Files selected for processing (4)

fuzz/src/router.rs (1 hunks)
lightning/src/ln/channel.rs (5 hunks)
lightning/src/ln/channelmanager.rs (6 hunks)
lightning/src/routing/router.rs (2 hunks)

Files skipped from review as they are similar to previous changes (1)

lightning/src/ln/channel.rs

Additional comments: 7

fuzz/src/router.rs (1)

245-246: Initialization of pending_inbound_htlcs and pending_outbound_htlcs as empty vectors aligns with the PR's objective to enhance ChannelDetails functionality. This change is necessary for the fuzz test to compile and run successfully after the modifications to ChannelDetails. Ensure that all other necessary initializations or adjustments related to these new fields are also made throughout the test suite to maintain consistency.

lightning/src/routing/router.rs (2)

3288-3289: Initialization of pending_inbound_htlcs and pending_outbound_htlcs with empty vectors is correct and follows Rust conventions for initializing collections. This change aligns with the PR's objective to enhance ChannelDetails functionality by including details about pending HTLCs.

8431-8432: Similar to the previous comment, the addition of pending_inbound_htlcs and pending_outbound_htlcs fields with empty vector initializations in this context is appropriate. It ensures that the new fields are accounted for in struct initializations within both test and benchmark setups.

lightning/src/ln/channelmanager.rs (4)

1890-1891: Correctly initializes pending_inbound_htlcs and pending_outbound_htlcs in the ChannelDetails constructor. Ensure that the methods get_pending_inbound_htlc_details and get_pending_outbound_htlc_details are implemented efficiently and correctly.

9474-9475: The serialization of pending_inbound_htlcs and pending_outbound_htlcs uses optional_vec, which is appropriate for optional fields. Verify that this approach maintains backward compatibility and correctly handles empty vectors.

9514-9515: Similar to serialization, deserialization of pending_inbound_htlcs and pending_outbound_htlcs uses optional_vec. Ensure deserialization logic correctly handles cases where these fields are not present in older serialized data.

9552-9553: Properly handles the case where pending_inbound_htlcs and pending_outbound_htlcs are not present in the serialized data by defaulting to empty vectors. This ensures backward compatibility with data serialized by older versions.

coderabbitai · 2024-02-06T01:13:48Z

lightning/src/ln/channelmanager.rs

@@ -44,6 +44,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa
 // construct one themselves.
 use crate::ln::{inbound_payment, ChannelId, PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channel::{self, Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext};
+pub use crate::ln::channel::{InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails};


Re-exporting types from a module that is not publicly accessible outside the crate could lead to confusion. Consider moving these types to a more appropriate location if they are intended for public use.

coderabbitai · 2024-02-06T01:13:48Z

lightning/src/ln/channelmanager.rs

+	/// Pending inbound HTLCs.
+	///
+	/// This field is empty for objects serialized with LDK versions prior to 0.0.122.
+	pub pending_inbound_htlcs: Vec<InboundHTLCDetails>,
+	/// Pending outbound HTLCs.
+	///
+	/// This field is empty for objects serialized with LDK versions prior to 0.0.122.
+	pub pending_outbound_htlcs: Vec<OutboundHTLCDetails>,


Ensure that the handling of pending_inbound_htlcs and pending_outbound_htlcs fields in ChannelDetails is backward compatible and that these fields are properly documented, including their behavior in versions prior to 0.0.122.

wvanlint · 2024-02-06T01:25:34Z

Logic and variants all LGTM! A few small nits on documentation and whatnot, but feel free to address those + squash and I think we can land this 🎉

Thanks! 🙏 Squashed.

This exposes details around pending HTLCs in ChannelDetails. The state of the HTLC in the state machine is also included, so it can be determined which protocol message the HTLC is waiting for to advance.

TheBlueMatt · 2024-02-06T18:43:17Z

lightning/src/ln/channel.rs

+	/// The state of the HTLC in the state machine.
+	/// Determines on which commitment transactions the HTLC is included and what message the HTLC is
+	/// waiting for to advance to the next state.
+	/// See [`InboundHTLCStateDetails`] for information on the specific states.


Docs should say when this can be None.

v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan

wvanlint force-pushed the list_pending_htlcs branch from 67e872b to a31802a Compare July 21, 2023 22:46

wvanlint mentioned this pull request Jul 22, 2023

Expose HTLCStats in ChannelDetails #2349

Closed

wvanlint force-pushed the list_pending_htlcs branch from a31802a to 9ca0041 Compare July 22, 2023 02:07

wvanlint marked this pull request as ready for review July 24, 2023 17:24

wvanlint mentioned this pull request Jul 25, 2023

Expose HTLCs in ChannelDetails #2087

Open

valentinewallace reviewed Jul 26, 2023

View reviewed changes

wvanlint force-pushed the list_pending_htlcs branch from 9ca0041 to dbcd317 Compare July 27, 2023 20:28

wvanlint requested a review from valentinewallace July 27, 2023 21:38

wvanlint force-pushed the list_pending_htlcs branch from dbcd317 to dc6a109 Compare July 28, 2023 00:03

wvanlint force-pushed the list_pending_htlcs branch 3 times, most recently from 529da61 to a9bdb45 Compare August 5, 2023 22:48

wvanlint force-pushed the list_pending_htlcs branch from a9bdb45 to a8fd24c Compare August 5, 2023 23:04

TheBlueMatt reviewed Aug 8, 2023

View reviewed changes

valentinewallace reviewed Aug 8, 2023

View reviewed changes

wvanlint mentioned this pull request Aug 10, 2023

Remove AvailableBalances::balance_msat #2476

Merged

wvanlint force-pushed the list_pending_htlcs branch 5 times, most recently from 1a3a29f to d8efd56 Compare August 16, 2023 01:11

TheBlueMatt reviewed Aug 16, 2023

View reviewed changes

wvanlint force-pushed the list_pending_htlcs branch from d8efd56 to 7a44af3 Compare January 22, 2024 01:50

coderabbitai bot reviewed Jan 22, 2024

View reviewed changes

wvanlint force-pushed the list_pending_htlcs branch from 7a44af3 to 55335f1 Compare January 22, 2024 04:49

coderabbitai bot reviewed Jan 22, 2024

View reviewed changes

wvanlint force-pushed the list_pending_htlcs branch 3 times, most recently from 7a89973 to 44920ff Compare January 22, 2024 05:22

coderabbitai bot reviewed Jan 22, 2024

View reviewed changes

wvanlint force-pushed the list_pending_htlcs branch from 44920ff to 58e8e16 Compare January 23, 2024 19:08

wvanlint requested review from TheBlueMatt and valentinewallace January 23, 2024 19:09

coderabbitai bot reviewed Jan 23, 2024

View reviewed changes

coderabbitai bot reviewed Feb 1, 2024

View reviewed changes

TheBlueMatt previously approved these changes Feb 5, 2024

View reviewed changes

wvanlint dismissed TheBlueMatt’s stale review via bb85b33 February 6, 2024 01:12

wvanlint force-pushed the list_pending_htlcs branch from 98df707 to bb85b33 Compare February 6, 2024 01:12

coderabbitai bot reviewed Feb 6, 2024

View reviewed changes

Include pending HTLCs in ChannelDetails

67e788e

This exposes details around pending HTLCs in ChannelDetails. The state of the HTLC in the state machine is also included, so it can be determined which protocol message the HTLC is waiting for to advance.

wvanlint force-pushed the list_pending_htlcs branch from bb85b33 to 67e788e Compare February 6, 2024 01:36

TheBlueMatt approved these changes Feb 6, 2024

View reviewed changes

TheBlueMatt merged commit 4a7ba67 into lightningdevkit:main Feb 6, 2024
13 of 15 checks passed

wvanlint deleted the list_pending_htlcs branch February 6, 2024 19:02

acceleratesage mentioned this pull request Oct 10, 2024

Expose current dust exposure #2264

Open

	/// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we
	/// created it we would have put it in the holding cell instead). When they next revoke_and_ack
	/// we will promote to Committed (note that they may not accept it until the next time we
	/// revoke, but we don't really care about that:
	/// * they've revoked, so worst case we can announce an old state and get our (option on)
	/// money back (though we won't), and,
	/// * we'll send them a revoke when they send a commitment_signed, and since only they're
	/// allowed to remove it, the "can only be removed once committed on both sides" requirement
	/// doesn't matter to us and it's up to them to enforce it, worst-case they jump ahead but
	/// we'll never get out of sync).
	/// Note that we Box the OnionPacket as it's rather large and we don't want to blow up
	/// OutboundHTLCOutput's size just for a temporary bit

	/// See [InboundHTLCStateDetails] for information on the specific states.
	/// See [`InboundHTLCStateDetails`] for information on the specific states.

	/// This takes into account the second-stage HTLC transactions as well.
	/// For non-anchor channels, this takes into account the cost of the second-stage HTLC transactions as well.

	impl_writeable_tlv_based_enum!(InboundHTLCStateDetails,
	impl_writeable_tlv_based_enum_upgradable!(InboundHTLCStateDetails,

	/// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is be added
	/// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is added

		/// and we removed the HTLC from our commitment transaction through a commitment_signed and
		/// revoke_and_ack exchange. We are awaiting the appropriate revoke_and_ack's from the remote for

	/// See [OutboundHTLCStateDetails] for information on the specific states.
	/// See [`OutboundHTLCStateDetails`] for information on the specific states.

Include pending HTLC's in ChannelDetails #2442

Include pending HTLC's in ChannelDetails #2442

Conversation

wvanlint commented Jul 21, 2023 • edited Loading

codecov-commenter commented Jul 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Oct 31, 2023

wvanlint commented Nov 3, 2023 • edited Loading

wpaulino commented Nov 3, 2023

coderabbitai bot commented Jan 22, 2024 • edited Loading

Rate Limit Exceeded

Walkthrough

Changes

Poem

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

Conclusion

Conclusion

Verifying the review comment

Conclusion

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

wvanlint commented Jan 22, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

wvanlint commented Jul 21, 2023 •

edited

Loading

codecov-commenter commented Jul 22, 2023 •

edited

Loading

wvanlint Jan 22, 2024 •

edited

Loading

wvanlint Feb 1, 2024 •

edited

Loading

wvanlint commented Nov 3, 2023 •

edited

Loading

coderabbitai bot commented Jan 22, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)